Skip to content

fix(markdown-common): retain empty nodes arrays in AST to satisfy strict=true in concerto-v4#652

Merged
mttrbrts merged 1 commit intoaccordproject:mainfrom
rockaxorb13:fix/ast-empty-nodes
Apr 10, 2026
Merged

fix(markdown-common): retain empty nodes arrays in AST to satisfy strict=true in concerto-v4#652
mttrbrts merged 1 commit intoaccordproject:mainfrom
rockaxorb13:fix/ast-empty-nodes

Conversation

@rockaxorb13
Copy link
Copy Markdown
Contributor

Description

fixes #651
This PR fixes a critical AST validation crash that occurs when using Concerto v4 engine (specifically 4.0.0-beta.1 and later) alongside markdown-transform. I uncovered this bug 2-3 weeks ago and and the fix was easier than I anticipated!

The Root Cause:
As described in the issue:
In packages/markdown-common/lib/FromMarkdownIt.js (blockToCommonMark), the parser was explicitly deleting the nodes array if a markdown element (like an empty clause, list, or blockquote) had no children. While Concerto v3 permitted this "lazy AST" where the array was simply undefined, the new strict type-checking in Concerto v4 throws a fatal ValidationException: Expected property 'nodes' to be an array because nodes is defined as a mandatory array in the metamodel.

The Fix:
Removed the aggressive deletion block (delete node.nodes) in FromMarkdownIt.js so that empty elements correctly retain an empty array ("nodes": []).

Testing / Validation

  • Tested against Concerto v3: Fully backward compatible. Ran the full test suite and updated Jest snapshots successfully.
  • Tested against Concerto v4: Prevents the ValidationException crash in strict parsing environments.

Standalone Script Output Proof:
I made a script to directly test this:
By feeding an empty clause ({{#clause testClause}}\n{{/clause}}) into CiceroMarkTransformer, we can see the exact structural change to the AST:

Before (Fails v4 Validation):

--- RAW AST OUTPUT ---
{
  "$class": "org.accordproject.ciceromark@0.6.0.Clause",
  "name": "testClause"
}
❌ BUG REPRODUCED: 'nodes' array was deleted!

After (Passes v4 Validation):

--- RAW AST OUTPUT ---
{
  "$class": "org.accordproject.ciceromark@0.6.0.Clause",
  "name": "testClause",
  "nodes": []
}
✅ FIX WORKS: 'nodes' array is present!

…ict Concerto validation

Signed-off-by: Aadityavardhan Singh <singhrashmi018@gmail.com>
@rockaxorb13 rockaxorb13 force-pushed the fix/ast-empty-nodes branch from 5d1a87b to 6da1b6f Compare April 7, 2026 06:21
@DianaLease DianaLease requested review from dselman and mttrbrts April 8, 2026 12:13
Copy link
Copy Markdown
Member

@mttrbrts mttrbrts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find!

@mttrbrts mttrbrts enabled auto-merge (squash) April 10, 2026 18:01
@mttrbrts mttrbrts merged commit c375d9a into accordproject:main Apr 10, 2026
9 checks passed
@coveralls
Copy link
Copy Markdown

Coverage Report for CI Build 24067562976

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage decreased (-0.08%) to 72.194%

Details

  • Coverage decreased (-0.08%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 2613
Covered Lines: 1958
Line Coverage: 74.93%
Relevant Branches: 1156
Covered Branches: 763
Branch Coverage: 66.0%
Branches in Coverage %: Yes
Coverage Strength: 1467.86 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Concerto v4 Compatibility: Missing 'nodes' array in AST causes ValidationException

3 participants